fix(p2p): accept relay URLs containing a path - #587

@claude review the PR

Claude encountered an error after 34s —— View job

I'll analyze this and get back to you.

@claude review this PR

Claude encountered an error after 24s —— View job

I'll analyze this and get back to you.

@claude Review this PR

Claude finished @emlautarom1's task in 4m 34s —— View job

Review: fix(p2p): accept relay URLs containing a path

• Gather context (diff, issue Bug(p2p): --p2p-relays rejects URLs with a path #572, Charon Go source)
• Review crates/p2p/src/config.rs (new RelayAddr type)
• Review crates/p2p/src/bootnode.rs
• Review CLI changes (common.rs, run.rs, dkg.rs, relay.rs, test/peers.rs)
• Cross-check against Charon Go behavior
• Post review

Verdict: 👍 Looks good — approve with minor follow-ups

The root cause is correctly identified and the fix is the right shape. The old parse_relay_addr did multiaddr::from_url(relay).or_else(|_| Multiaddr::from_str(relay)); libp2p's from_url returns Err(InformationalLoss) for any URL carrying a path/query, and Multiaddr::from_str("http://…") then also fails — so --p2p-relays=http://relay:3640/enr was rejected outright. That's a real endpoint: crates/relay-server/src/web.rs:139 registers /enr alongside / (:138). Replacing the lossy multiaddr round-trip with a RelayAddr { Url | Multiaddr } sum type is the correct structural fix, not a patch.

Charon parity checks I ran (charon/p2p/bootnode.go:32-34, charon/cmd/run.go:185-195):

Note this PR moves relay validation earlier than Go does (flag-parse time vs. NewRelays). Net effect is the same — the command fails either way — and the error message is much better (parse relay address 'not-an-address': invalid relay multiaddr: … vs. the old Invalid multiaddr: invalid multiaddr). Good change.

Test coverage is genuinely good: the wiremock stub in crates/p2p/src/bootnode.rs mounts the body at a specific path so a request that loses the path 404s rather than silently succeeding. That's the right way to test this regression.

Findings

1. test peers still probes the raw flag strings — --p2p-relays="" now yields a bogus test target

crates/cli/src/commands/test/peers.rs:316 still passes &args.p2p_relays (raw Vec<String>) to run_relay_http_tests, while :282 parses the same input into relay_addrs. The two now disagree on the empty case:

• Before: --p2p-relays="" → new_relays(&[""]) → Multiaddr::from_str("") succeeds as a zero-component multiaddr → PeerFromMultiaddr(""), command fails loudly.
• After: parse_relay_addrs correctly returns no relays, but run_relay_http_tests(&[""]) still runs relay_ping_test("") (:488) and emits a target keyed "relay " with a reqwest failure — a fabricated failing result for a relay the user explicitly disabled.

Separately (pre-existing, but now trivially fixable): a raw multiaddr relay like /ip4/10.0.0.1/tcp/3610/p2p/… gets HTTP-pinged as if it were a URL.

Both fall out if the function takes &[RelayAddr] and filters to the Url variant:

let mut futs: FuturesUnordered<_> = relay_addrs
    .iter()
    .filter_map(|r| match r {
        RelayAddr::Url(url) => Some(url.clone()),
        RelayAddr::Multiaddr(_) => None,
    })
    …

Fix this →

2. RelayAddrError::Scheme is unreachable in the tests

crates/p2p/src/config.rs:33 — the Scheme variant is reachable (httpx://relay takes the http-prefix branch, parses as a URL, then fails the scheme match), but relay_addr_rejects_invalid_forms only covers Url(_), Multiaddr(_) and Empty. The ftp:// case in that test exercises the multiaddr branch, not Scheme. Worth one more assertion so the variant doesn't rot.

3. Repeated-flag edge case diverges from Go (low)

--p2p-relays="" --p2p-relays=https://x gives clap ["", "https://x"] → rejected by parse_relay_addrs, since the lone-empty guard is if let [only]. Go's pflag replaces on first Set and appends after, yielding ["https://x"]. Obscure enough that I wouldn't block on it, but the doc comment on parse_relay_addrs (crates/cli/src/commands/common.rs:57-60) claims to describe the full empty-value contract and doesn't mention this case.

4. Nits

• crates/p2p/src/bootnode.rs — in the RelayAddr::Url(url) arm you call relay_addr.is_insecure_url(), which re-matches the enum you just destructured. url.scheme() != "https" is more direct and doesn't depend on a helper whose contract is about the outer type.
• crates/p2p/src/config.rs:69 — is_insecure_url's doc says "Returns true for a plain-http:// URL". It's also (deliberately) false for every Multiaddr, which is the parity-relevant half. The test asserts it; the doc should say it.
• crates/p2p/src/bootnode.rs — BootnodeError::RelayQueryFailed is now returned for both a non-200 status and a JSON parse failure, dropping the serde_json::Error (it's only logged). Since you're already touching this enum, attaching the cause as #[source] would make the retry-loop failures diagnosable from the error chain, matching what you did for RelayAddrError.
• crates/p2p/src/config.rs — RelayAddr::Url(url::Url) and RelayAddrError::Url(url::ParseError) put url in pluto-p2p's public API. Fine here (workspace-internal), just noting it's now a semver-relevant dependency.

Not verified

I could not run cargo clippy --workspace --all-targets --all-features, cargo test --workspace --all-features, or cargo +nightly fmt --check — those commands require approval in this environment, so I reviewed statically. One spot to double-check under fmt: the reformatted match query_relay_addrs(…).await { block in resolve_relay (crates/p2p/src/bootnode.rs) has an unusual brace placement.

None of the above blocks the fix — the core change is sound and well-tested. #1 is the only one I'd want addressed before merge.
· feat/fix-p2p-relay-url